Skip to content

fix: js thread lock on scene lifecycle - #8702

Merged
lorux0 merged 21 commits into
devfrom
fix/scene-lifecycle
May 14, 2026
Merged

fix: js thread lock on scene lifecycle#8702
lorux0 merged 21 commits into
devfrom
fix/scene-lifecycle

Conversation

@lorux0

@lorux0 lorux0 commented May 8, 2026

Copy link
Copy Markdown
Collaborator

Fixes #8654 and #8493

Problem

The world bloomgarden.dcl.eth sometimes gets stuck at await runtimeInstance.UpdateScene(deltaTime);. The JS thread is locked. The explorer did not recover from that, keeping the scene state as "running code", being unable to dispose it on teleport.
The REAL reason is obscure, but i manage to conclude that it breaks 100% when the delta time passed to the scene is large, ie: 3.9, when it normally is 0.025. This happens especially after the loading screen hides and the scene renders, which sometimes provokes a hiccup.
Most likely there is an infinite loop in the scene code. I've tried making a simple scene with an endless loop and it has the same symptom.

Solution

  • Hardens the scene lifecycle against stuck JS execution.
  • Add a new state into scenes: Starting. Prevents possible race conditions between main thread, JS thread executions and scene state.
  • Adds a watchdog that interrupts V8 when JS hangs past a threshold. Mitigates issues on JS code.
  • Caps delta time fed to JS, preventing disrupting execution paths on JS.

Test Steps

The reproduction rate of these issues is very low. Only achieved breaking it by forcing the code.
Try starting the explorer at bloomgarden.dcl.eth. Play a bit.
Perform teleport operations. Perform /reloads. Move around the world.
Check that scenes runs normally.
Check portable experiences too.

Quality Checklist

  • Changes have been tested locally
  • Documentation has been updated (if required)
  • Performance impact has been considered
  • For SDK features: Test scene is included

Code Review Reference

Please review our Branch & PR Standards before submitting. It explains the automated review flow, QA/DEV approval requirements, and what each label does — especially useful for first-time contributors.

@lorux0
lorux0 requested review from a team as code owners May 8, 2026 18:28
@github-actions

github-actions Bot commented May 8, 2026

Copy link
Copy Markdown
Contributor

@claude

claude Bot commented May 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @lorux0's task in 6m 1s —— View job


Claude PR Review

  • Read CLAUDE.md and project docs
  • Review the full PR diff
  • Check root cause vs symptom
  • Identify blocking issues
  • Post final review

Summary

Problem: bloomgarden.dcl.eth (and similar) sometimes lock the JS thread inside await runtimeInstance.UpdateScene(deltaTime), leaving the scene perpetually in Running and undisposable on teleport. Repro is forced; the user-side trigger is a large dt (~3.9 s) seen right after the loading screen hides.

Root-cause check: PR explicitly admits the underlying JS deadlock cause is unknown. The diff is a layered defensive fix:

  1. MAX_DELTA_TIME = 0.1f clamp — symptomatic; mitigates the known trigger (large dt → user-JS spin loops). Reasonable as a guardrail.
  2. Interrupt() watchdog (10s update / 30s start) — recovery mechanism, not a fix; force-unwinds V8 to break the stuck state. Appropriate given the cause is unknown.
  3. StartUpdateLoopAsync split into StartAsync + UpdateLoopAsync — addresses a real ordering bug: scenes were registered in IScenesCache before onStart finished, opening a race where the cache could observe a scene that's still initializing.

So #3 is a true root-cause fix; #1 and #2 are defense-in-depth. Acceptable framing.


Blocking

  1. Leftover debug logs in SetIsCurrent (SceneFacade.cs:291, :297) — two ReportHub.Log(ReportCategory.ALWAYS, …) calls bracket the body and fire on every isCurrent toggle. With many scenes loaded these spam production logs. Remove them. (Inline comment on :291 did not anchor — please address both lines.)
  2. Stray // // comment marker in ControlSceneUpdateLoopSystem.cs:126 — see inline.
  3. Watchdog can overwrite a Disposed/Disposing state with JavaScriptErrorSceneFacade.cs:246. Guard with IsNotRunningState() before setting. See inline.

Non-blocking concerns

  1. Watchdog CTS in StartAsync is not linked to caller ctSceneFacade.cs:131. UpdateLoopAsync does this correctly (:167). See inline.
  2. Race: watchdog can interrupt a tick that has just finished — see inline at SceneFacade.cs:237. Threshold (10 s) is generous enough that real ticks won't trip this in practice, but a re-read of tickStartTimestamp after the elapsed check would close the window cleanly.
  3. OperationCanceledException reported as exception in ControlSceneUpdateLoopSystem.cs:132-135 — violates CLAUDE.md §9. Was already broad before, but more cancellable awaits flow through it now.
  4. ct parameter in StartAsync is unusedruntimeInstance.StartScene() is non-cancellable; the watchdog is the only escape. Worth either propagating where possible or dropping the parameter to remove the misleading signal.
  5. First tick may use stale dt — first iteration of UpdateLoopAsync runs with deltaTime = 0, then on the second iteration deltaTime is computed from the prior idle/sleep window. The clamp at 0.1f fully covers this; flagging only because the inline comment refers to "first iteration" specifically.
  6. scenesCache add now happens after an async hop — by design, but if the entity is destroyed during the await the scene is added to the cache and immediately needs to be cleaned up via the ECS DeleteEntityIntention path. Worth a smoke test on rapid teleports to confirm no leaks.

Tests

Test updates correctly track the API rename and split. No coverage added for the watchdog itself (interrupt path, threshold trip, state transition guards) — would be nice but not blocking.


REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Reworks scene runtime lifecycle (StartAsync/UpdateLoopAsync split), adds a new V8 interrupt path on ISceneRuntime, changes async ordering between JS init and IScenesCache registration, and shifts cancellation/threading boundaries in ControlSceneUpdateLoopSystem.
QA_REQUIRED: YES
• Branch: fix/scene-lifecycle

@claude

claude Bot commented May 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @lorux0's task in 5m 28s —— View job


Claude PR Review

  • Read CLAUDE.md and project docs
  • Review the full PR diff
  • Check root cause vs symptom
  • Identify blocking issues
  • Post final review

Summary

Problem this PR solves: scenes (notably bloomgarden.dcl.eth) sometimes get stuck inside await runtimeInstance.UpdateScene(deltaTime) after a hiccup feeds a large delta time (~3.9s) into JS. The scene state stays at Running and dispose-on-teleport never fires.

Root cause vs symptom: the PR is explicit that the underlying V8/JS hang has an obscure cause. The three changes are layered defenses:

  1. Lifecycle split (StartAsync + UpdateLoopAsync, cache add only after onStart) — this is a real ordering fix, not a workaround. It removes the prior race where scenesCache.Add ran synchronously on main while JS startup was kicked off in parallel.
  2. V8 hang watchdog — proper escape valve: marks state JavaScriptError, calls engine.Interrupt(), lets the existing ScriptEngineException catch unwind. Sound design.
  3. MAX_DELTA_TIME cap (0.1f) — pragmatic mitigation for the observed trigger; doesn't fix why JS spins on large dt, but the SDK contract was never to feed multi-second steps.

Net assessment: not a "swallow the exception" fix. The lifecycle split addresses ordering directly; the watchdog is a defensive layer with a clear failure path; the dt cap is honestly labeled as mitigation.


Blocking issues

  1. SceneFacade.cs:228RunHangWatchdogAsync only catches OperationCanceledException on DCLTask.Delay. This is a detached UniTaskVoid started via .Forget(). Per CLAUDE.md async rule 9, the body must catch any other exception and report via ReportHub.LogException. Today, an exception from Interlocked.Read, the format string, or runtimeInstance.Interrupt() (outside the inner catch) becomes unobserved. (See inline comment.)

  2. ControlSceneUpdateLoopSystem.cs:113-122 — race between the IsNotRunningState() check and scenesCache.Add. The check runs on the thread pool before await UniTask.SwitchToMainThread(...). During that gap, a concurrent Dispose / DisposeAsync / watchdog interrupt can flip state. We then add a disposing/errored scene to the cache. Re-check after each SwitchTo* await. (See inline comment.)

  3. ControlSceneUpdateLoopSystem.cs:126 — comment typo. // // 3. Run the update loop on the thread pool has a stray //, and the step is actually 4 (cache add is 3).

Non-blocking observations

  • SceneFacade.cs:132StartAsync watchdog uses new CancellationTokenSource() instead of CreateLinkedTokenSource(ct) (UpdateLoopAsync uses linked). Inconsistent; minor. (Inline comment.)
  • MAX_DELTA_TIME = 0.1f is undocumented as a value choice. Comment explains why a cap exists but not why 0.1s specifically. A line noting "≥10fps minimum step" would help future readers.
  • Behavior change worth flagging for QA: the scene is no longer in scenesCache until onStart completes. Anything elsewhere that assumed "cache contains the scene as soon as the entity has ISceneFacade" will now see a delay. Worth a sweep — at minimum, exercise teleport / reload / portable experiences, which is what the test plan calls out.

REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches scene runtime lifecycle, V8 engine interrupt path, ECS scene-cache registration ordering, and detached UniTaskVoid flows — all listed as complex subsystems.
QA_REQUIRED: YES
• Branch: fix/scene-lifecycle

Comment thread Explorer/Assets/DCL/Infrastructure/SceneRunner/SceneFacade.cs
Comment thread Explorer/Assets/DCL/Infrastructure/SceneRunner/SceneFacade.cs Outdated
Comment thread Explorer/Assets/DCL/Infrastructure/SceneRunner/SceneFacade.cs
@github-actions

github-actions Bot commented May 8, 2026

Copy link
Copy Markdown
Contributor

🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging.

Comment thread Explorer/Assets/DCL/Infrastructure/SceneRunner/SceneFacade.cs
Comment thread Explorer/Assets/DCL/Infrastructure/SceneRunner/SceneFacade.cs Outdated
@claude

claude Bot commented May 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @lorux0's task in 3m 24s —— View job


Claude PR Review

  • Read CLAUDE.md and project docs
  • Review the full PR diff (post-synchronize)
  • Verify previously-flagged blocking issues
  • Check root cause vs symptom
  • Identify blocking issues
  • Post final review

Summary

Problem: scenes (notably bloomgarden.dcl.eth) sometimes get stuck inside await runtimeInstance.UpdateScene(deltaTime) after a hiccup feeds a multi-second dt into JS. The scene state stays at Running and dispose-on-teleport never fires.

Root cause vs symptom: the PR is honest that the underlying V8/JS hang has an obscure cause. The diff is layered:

  1. Lifecycle split (StartAsync + UpdateLoopAsync, cache add only after onStart) — real ordering fix. Removes the prior race where scenesCache.Add ran before JS init completed.
  2. V8 hang watchdog — escape valve: marks state JavaScriptError, calls engine.Interrupt(), lets the ScriptEngineException catch unwind. Sound design.
  3. MAX_DELTA_TIME = 0.1f cap — pragmatic mitigation for the observed trigger. Doesn't fix why JS spins on large dt, but the SDK contract was never to feed multi-second steps.

Net assessment: not a swallow-the-exception fix. Lifecycle split is a true root-cause fix; watchdog and dt cap are defensive layers honestly labeled as such.

Since the previous review the only commit (69f61d62 remove logs) addressed the debug-log spam in SetIsCurrent. All other previously-flagged blocking issues remain.


Blocking issues (all carried over from prior review)

  1. SceneFacade.cs:228-256RunHangWatchdogAsync lacks a top-level catch. Detached UniTaskVoid started via .Forget(); only DCLTask.Delay is guarded. Violates CLAUDE.md §9. (Inline comment.)
  2. ControlSceneUpdateLoopSystem.cs:113→122 — race between IsNotRunningState() check and scenesCache.Add. The await UniTask.SwitchToMainThread between the check and the cache write opens a window for a concurrent Dispose/DisposeAsync or watchdog interrupt to flip state, after which we still publish a non-running scene to scenesCache. Re-check after each thread-switch await. (Inline comment.)
  3. SceneFacade.cs:247 — watchdog can overwrite Disposing/Disposed with JavaScriptError. Guard the state set with IsNotRunningState() so the watchdog never overrides a terminal disposal state. (Inline comment.)
  4. ControlSceneUpdateLoopSystem.cs:134OperationCanceledException reported as exception. With destroyCancellationToken flowing through SwitchToMainThread/SwitchToThreadPool/UpdateLoopAsync, normal shutdown cancellations now log as exceptions. Violates CLAUDE.md §9. (Inline comment.)
  5. ControlSceneUpdateLoopSystem.cs:126 — stray // // comment marker plus wrong step number (cache add at line 122 is step 3, so this should be step 4). (Inline comment.)

Non-blocking observations

  • SceneFacade.cs:132StartAsync watchdog uses a standalone new CancellationTokenSource() while UpdateLoopAsync uses CreateLinkedTokenSource(ct). Inconsistent and slightly slower teardown on outer cancellation. (Inline comment.)
  • Watchdog tick race (SceneFacade.cs:235-247): if a tick takes ~9.9s and finishes between the elapsed read and the Set(JavaScriptError)/Interrupt(), the next tick is interrupted on a healthy scene. Threshold is generous so unlikely; closing it cleanly would re-read tickStartTimestamp and confirm it equals startTs before the interrupt.
  • MAX_DELTA_TIME = 0.1f — comment explains why a cap exists but not why 0.1 specifically. A line noting "≥10fps minimum step" would help future readers.
  • First tick uses deltaTime = 0 — covered by the cap; just flagging because the comment refers to "first iteration" specifically.
  • QA note: scenes are no longer in scenesCache until onStart completes. Anything that assumed "cache contains scene as soon as the entity has ISceneFacade" will see a delay — exercise teleport / /reload / portable experiences.

Tests

Test API was updated to track the rename and split. No coverage added for the watchdog itself (interrupt path, threshold trip, state-transition guard) — would be useful but not blocking.


REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Reworks scene runtime lifecycle (StartAsync/UpdateLoopAsync split), adds a new V8 interrupt path on ISceneRuntime, changes async ordering between JS init and IScenesCache registration, and shifts cancellation/threading boundaries in ControlSceneUpdateLoopSystem.
QA_REQUIRED: YES
• Branch: fix/scene-lifecycle

Comment thread Explorer/Assets/DCL/Infrastructure/SceneRunner/SceneFacade.cs Outdated
Comment thread Explorer/Assets/DCL/Infrastructure/SceneRunner/SceneFacade.cs
Comment thread Explorer/Assets/DCL/Infrastructure/SceneRunner/SceneFacade.cs Outdated
@claude

claude Bot commented May 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @lorux0's task in 3m 41s —— View job


Claude PR Review

  • Read CLAUDE.md and project docs
  • Review the full PR diff (post-synchronize)
  • Verify previously-flagged blocking issues
  • Check root cause vs symptom
  • Identify blocking issues
  • Post final review

Summary

Problem: scenes (notably bloomgarden.dcl.eth) sometimes get stuck inside await runtimeInstance.UpdateScene(deltaTime) after a hiccup feeds a multi-second dt into JS. The scene state stays at Running and dispose-on-teleport never fires.

Root cause vs symptom: the PR is honest that the underlying V8/JS hang has an obscure cause. The diff is a layered defense:

  1. Lifecycle split (StartAsync + UpdateLoopAsync, cache add only after onStart) — real ordering fix. Removes the prior race where scenesCache.Add ran before JS init completed.
  2. V8 hang watchdog — escape valve: marks state JavaScriptError, calls engine.Interrupt(), lets the ScriptEngineException catch unwind. Sound design.
  3. MAX_DELTA_TIME = 0.1f cap — pragmatic mitigation for the observed trigger. Doesn't fix why JS spins on large dt, but the SDK contract was never to feed multi-second steps.

Net assessment: not a swallow-the-exception fix. Lifecycle split is a true root-cause fix; watchdog and dt cap are defensive layers honestly labeled as such.

Since the previous review (f837796e review fixes):

  • ✅ Watchdog UniTaskVoid now has a top-level catch that ignores OperationCanceledException and reports any other exception via ReportHub.LogException.
  • ✅ Watchdog state set is now guarded with if (!IsNotRunningState()) so it won't overwrite Disposed/Disposing.
  • StartAsync's watchdogCts is now linked to the caller's ct via CreateLinkedTokenSource(ct).
  • ✅ Stray // // comment marker fixed.

Blocking issues (still unaddressed)

  1. ControlSceneUpdateLoopSystem.cs:113→117→122 — race between IsNotRunningState() check and scenesCache.Add. The await UniTask.SwitchToMainThread(cancellationToken: destroyCancellationToken) at line 117 is a real yield; during it a concurrent Dispose / DisposeAsync on the main thread (or watchdog Interrupt) can flip state, after which we still publish a non-running scene into scenesCache. Re-check IsNotRunningState() after each thread-switch await. (Inline comment.)

  2. ControlSceneUpdateLoopSystem.cs:132-135OperationCanceledException reported as exception. Violates CLAUDE.md §9. destroyCancellationToken flows through UniTask.SwitchToMainThread (line 117), which throws OperationCanceledException on cancellation; the broad catch (Exception) then reports normal shutdown cancellations to production logs. Add a catch (OperationCanceledException) { } clause before the general catch. (Inline comment.)

Non-blocking observations

  • Step number comment on ControlSceneUpdateLoopSystem.cs:126 (// 3. Run the update loop on the thread pool) is correct — steps in the file are 1 (line 101), 2 (line 116), 3 (line 126). My prior review claim that this should be "step 4" was wrong; please disregard. ✅
  • Watchdog tick race (SceneFacade.cs:236-254): if a tick takes ~9.9s and finishes between the elapsed read and the Interrupt()/state set, the next tick is interrupted on a healthy scene. Threshold is generous so unlikely; closing it cleanly would re-read tickStartTimestamp and confirm it equals startTs before issuing the interrupt.
  • MAX_DELTA_TIME = 0.1f — comment explains why a cap exists but not why 0.1 specifically. A line noting "≥10fps minimum step" would help future readers.
  • First tick uses deltaTime = 0 — covered by the cap; just flagging because the watchdog comment refers to "first iteration" specifically.
  • QA note: scenes are no longer in scenesCache until onStart completes. Anything that assumed "cache contains scene as soon as the entity has ISceneFacade" will see a delay — exercise teleport / /reload / portable experiences.

Tests

Test API was updated to track the rename and split. No coverage added for the watchdog itself (interrupt path, threshold trip, state-transition guard) — would be useful but not blocking.


REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Reworks scene runtime lifecycle (StartAsync/UpdateLoopAsync split), adds a new V8 interrupt path on ISceneRuntime, changes async ordering between JS init and IScenesCache registration, and shifts cancellation/threading boundaries in ControlSceneUpdateLoopSystem.
QA_REQUIRED: YES
• Branch: fix/scene-lifecycle

@lorux0
lorux0 requested a review from mikhail-dcl May 12, 2026 14:07
@claude

claude Bot commented May 13, 2026

Copy link
Copy Markdown
Contributor

Claude finished @lorux0's task in 4m 22s —— View job


Claude PR Review

  • Read CLAUDE.md and project docs
  • Read changed files and diff
  • Verify previously-flagged blocking issues
  • Check new approach: Starting state + ECS system guards
  • Identify blocking issues
  • Post final review

Summary

Problem: scenes (notably bloomgarden.dcl.eth) sometimes get stuck inside await runtimeInstance.UpdateScene(deltaTime) after a hiccup feeds a multi-second dt into JS. Scene stays at Running and dispose-on-teleport never fires.

Root cause vs symptom: PR is honest that the underlying V8/JS hang is obscure. The diff is layered:

  1. Starting state + synchronous cache add — real ordering fix. Cache add now happens synchronously on the main thread before the thread switch. State is set to Starting before thread switch, so every consumer that polls IScenesCache or State sees a consistent view. Previous iteration's startState snapshot race is resolved.
  2. V8 hang watchdog — escape valve: marks state JavaScriptError, calls engine.Interrupt().
  3. MAX_DELTA_TIME = 0.1f cap — pragmatic mitigation for the observed trigger.

Since the previous review (last failing on the startState snapshot race):

  • ✅ The startState snapshot race is fully resolved. State is now set to Starting synchronously on the main thread before DCLTask.SwitchToThreadPool(), so no async gap between "in cache" and "has defined state."
  • ✅ All previously-flagged DCLInterlocked migration issues resolved (Exchange(ref long, long) added, all accesses use DCLInterlocked).
  • OperationCanceledException handled correctly in ControlSceneUpdateLoopSystem.
  • ✅ Watchdog UniTaskVoid has top-level catch.
  • ✅ Watchdog guards Set(JavaScriptError) with !IsNotRunningState().
  • PlayerCRDTEntitiesHandlerSystem guards on State == Running.
  • SyncedGroup allows Starting OR Running to run scene ECS systems.
  • SceneReadinessReportQueue / GlobalDeferredLoadingSystem gate on State == Running.
  • CharacterPlatformUpdateSceneTickSystem / PlatformSaveLocalPosition gate on Running.

Blocking issues

1. SceneFacade.cs:183 — Unobserved ThreadStateException when StartScene() throws.

If runtimeInstance.StartScene() throws a ScriptEngineException (including ScriptInterruptedException from the 30-second start watchdog), sceneExceptionsHandler.OnJavaScriptException may set state to JavaScriptError. The finally block runs correctly, but then await UpdateLoopAsync(ct) is called with state JavaScriptError. UpdateLoopAsync checks State != SceneState.Starting → true → throws ThreadStateException("did not start!").

Since StartUpdateLoopAsync is invoked via .Forget() in ControlSceneUpdateLoopSystem, this exception is unobserved and will surface via UniTaskScheduler.UnobservedTaskException — logged as a production error — exactly when the 30s start watchdog fires, the primary scenario this PR is hardening against.

Fix: guard before the call:

if (SceneStateProvider.State != SceneState.Starting)
    return;

await UpdateLoopAsync(ct);

(See inline comment on line 183.)

Non-blocking observations

  • GlobalDeferredLoadingSystem.cs:89scenesCache.Contains(teleportParcel.Parcel) is dead code: TryGetByParcel already proved the parcel is present. See inline suggestion.
  • IsNotRunningState() does not include Starting — intentional; Starting is a valid active state and the watchdog correctly fires when stuck in Starting.
  • Watchdog tick race (previous review, author acknowledged as edge-case) — acceptable given the generous 10s/30s thresholds.
  • MAX_DELTA_TIME = 0.1f — comment explains why a cap exists; 0.1s = minimum 10fps step is reasonable.

REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Reworks scene runtime lifecycle (new Starting state, Starting/Running split in SyncedGroup), adds a V8 interrupt path, touches async ordering between JS init and IScenesCache registration, and modifies multiple ECS systems and scene readiness reporting.
QA_REQUIRED: YES

• Branch: fix/scene-lifecycle

Comment thread Explorer/Assets/DCL/Infrastructure/SceneRunner/SceneFacade.cs
Comment thread Explorer/Assets/DCL/ECS/GlobalPartitioning/GlobalDeferredLoadingSystem.cs Outdated
@claude

claude Bot commented May 13, 2026

Copy link
Copy Markdown
Contributor

Claude finished @lorux0's task in 4m 57s —— View job


Claude finished @lorux0's task


Claude PR Review

  • Read CLAUDE.md and project docs
  • Review the full PR diff (post-synchronize, latest commits bb3f3a22, f85c1d5c)
  • Verify previously-flagged blocking issue (SceneFacade.cs:183 — unobserved ThreadStateException)
  • Check changes introduced by bb3f3a22 fix conflict
  • Identify remaining blocking issues
  • Post final review

Summary

Problem: scenes (notably bloomgarden.dcl.eth) sometimes get stuck inside await runtimeInstance.UpdateScene(deltaTime) after a hiccup feeds a multi-second dt into JS. Scene stays at Running and dispose-on-teleport never fires.

Root cause vs symptom: PR is honest that the underlying V8/JS hang is obscure. The diff is layered:

  1. Starting state + synchronous cache add — real ordering fix. Cache add happens on the main thread before the thread switch. State transitions NotStartedStartingRunning as the scene progresses through init. All consumers gate on the appropriate state.
  2. V8 hang watchdog — escape valve: marks state JavaScriptError, calls engine.Interrupt().
  3. MAX_DELTA_TIME = 0.1f cap — pragmatic mitigation for the observed trigger.

Since the previous review (commits bb3f3a22 fix conflict, f85c1d5c review fixes):

  • Last blocking issue resolvedUpdateLoopAsync now returns early (return;) instead of throwing ThreadStateException when state is not Starting after a watchdog-interrupted StartScene(). This prevents an unobserved exception from a .Forget()-ed task (SceneFacade.cs:190).
  • Redundant Contains check removedGlobalDeferredLoadingSystem.cs:88 dead-code inner if removed.
  • SetTargetFPS body bug fixed (bb3f3a22) — Arrow expression body => with two statements silently dropped RuntimeMetrics.TargetFps = fps;. Fixed to a proper block body (SceneFacade.cs:119-123).
  • DCLInterlocked.Exchange(ref long, long) added and all tickStartTimestamp accesses migrated.
  • ✅ Watchdog UniTaskVoid has top-level try/catch (ignores OperationCanceledException, reports others).
  • ✅ Watchdog guards Set(JavaScriptError) with !IsNotRunningState().
  • StartAsync watchdog CTS uses CreateLinkedTokenSource(ct).
  • OperationCanceledException caught separately in ControlSceneUpdateLoopSystem.
  • SyncedGroup allows Starting OR Running to execute ECS systems.
  • ✅ All consumer systems gate on Running (PlayerCRDTEntitiesHandlerSystem, CharacterPlatformUpdateSceneTickSystem, PlatformSaveLocalPosition, GlobalDeferredLoadingSystem, SceneReadinessReportQueue).

Blocking issues

None.

Non-blocking observations

  • StartUpdateLoopAsync now .Forget()-ed in ControlSceneUpdateLoopSystem.cs:118: If Dispose() somehow runs between scenesCache.Add (line 103) and the call to StartUpdateLoopAsync (line 118), the State != NotStarted guard at SceneFacade.cs:148 would throw a ThreadStateException as an unobserved faulted UniTask. This race requires DeleteEntityIntention to be added within the thread-pool-switch latency (~microseconds) of a freshly-consumed scene — effectively impossible in practice — but worth noting as an architectural edge case.
  • GlobalDeferredLoadingSystem.cs:88: teleport fast-path now gates on Running (not Starting). A scene whose JS init just started won't suppress asset downloads during teleport. This is correct — we should wait for the scene to be fully ready.
  • Watchdog tests — no coverage added for the watchdog itself (interrupt path, threshold trip, state-transition guard). Not blocking.

REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Reworks scene runtime lifecycle (new Starting state, Starting/Running split in SyncedGroup), adds a V8 interrupt path, touches async ordering between JS init and IScenesCache registration, and modifies multiple ECS systems and scene readiness reporting.
QA_REQUIRED: YES

• Branch: fix/scene-lifecycle

@github-actions

Copy link
Copy Markdown
Contributor

🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging.

@claude

claude Bot commented May 14, 2026

Copy link
Copy Markdown
Contributor

Claude finished @lorux0's task in 5m 42s —— View job


Claude PR Review

  • Read CLAUDE.md and project docs
  • Review full PR diff (post-synchronize, latest commits 546a1857, ceb66dc1)
  • Verify previously-flagged blocking issues
  • Check new changes since last review
  • Post final review

Summary

Problem: scenes (notably bloomgarden.dcl.eth) sometimes get stuck inside await runtimeInstance.UpdateScene(deltaTime) after a hiccup feeds a multi-second dt into JS. Scene stays at Running and dispose-on-teleport never fires.

Root cause vs symptom: PR is honest that the underlying V8/JS hang is obscure. The diff is layered:

  1. New Starting state + synchronous cache add — real ordering fix. Cache add now happens synchronously on the main thread before the JS init thread switch. State transitions NotStarted → Starting → Running as the scene progresses through init.
  2. V8 hang watchdog — escape valve: marks state JavaScriptError, calls engine.Interrupt(), lets the ScriptEngineException catch unwind.
  3. MAX_DELTA_TIME = 0.1f cap — pragmatic mitigation for the observed trigger.

Since the previous PASS review (commits bb3f3a22 fix conflict, f85c1d5c review fixes):

  • 546a1857 fix player crdt tests: Adds correct SceneState.Running guard in PlayerCRDTEntitiesHandlerSystem.RemoveComponent — skips posting DeleteEntityIntention to a Disposing/Disposed/error scene world that races against teardown. Four new tests added to validate Starting/Disposing state handling: NotAssignPlayerWhenSceneIsStarting, AssignPlayerWhenSceneTransitionsFromStartingToRunning, SkipSceneSideCleanupWhenPreviousSceneIsDisposing, NotAssignPlayerWhenSceneIsDisposingFromTheStart. All tests are correct and exercise the right invariants.
  • ceb66dc1 Merge branch 'dev' into fix/scene-lifecycle — dev merge; only unrelated changes (spring bones, sidebar help menu, etc.) in the merge delta.

All previously-resolved blocking issues remain fixed:

  • UpdateLoopAsync returns early instead of throwing ThreadStateException when state is not Starting after watchdog-interrupted StartScene()
  • ✅ Watchdog UniTaskVoid has top-level try/catch
  • ✅ Watchdog guards Set(JavaScriptError) with !IsNotRunningState()
  • StartAsync watchdog CTS uses CreateLinkedTokenSource(ct)
  • ✅ All tickStartTimestamp accesses migrated to DCLInterlocked
  • OperationCanceledException caught separately in ControlSceneUpdateLoopSystem
  • SyncedGroup allows Starting || Running to execute ECS systems
  • ✅ All consumer systems gate on Running for player assignment, readiness reporting, and deferred loading
  • GlobalDeferredLoadingSystem dead Contains check removed
  • SetTargetFPS body bug fixed

Blocking issues

None.

Non-blocking observations

  • ControlSceneUpdateLoopSystem.cs:118scene.StartUpdateLoopAsync(...).Forget() on the thread pool. Unobserved exceptions escape through UniTaskScheduler.UnobservedTaskException rather than the catch (Exception e) wrapper. Acceptable for endless-loop tasks; the watchdog and internal catches provide the safety net.
  • PlayerCRDTEntitiesHandlerSystem.cs:124 — cleanup skipped when scene is JavaScriptError: the player entity in the scene world persists until the scene tears down. This is intentional (writing to an error-state world races with teardown), and the scene world is disposed shortly after, so no permanent leak.
  • QA note: scenes are no longer in scenesCache with Running state until onStart completes. Confirm teleport / /reload / portable experiences do not regress.
  • Watchdog tests — no coverage added for the watchdog itself (interrupt path, threshold trip, state-transition guard). Not blocking.

REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Reworks scene runtime lifecycle (new Starting state, Starting/Running split in SyncedGroup), adds a V8 interrupt path, touches async ordering between JS init and IScenesCache registration, and modifies multiple ECS systems and scene readiness reporting.
QA_REQUIRED: YES

• Branch: fix/scene-lifecycle

@mikhail-dcl mikhail-dcl left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just fix that second disconnected task

Comment thread Explorer/Assets/DCL/Infrastructure/SceneRunner/SceneFacade.cs
@github-actions

Copy link
Copy Markdown
Contributor

🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging.

@lorux0
lorux0 requested a review from mikhail-dcl May 14, 2026 14:32
@claude

claude Bot commented May 14, 2026

Copy link
Copy Markdown
Contributor

Claude finished @lorux0's task in 2m 39s —— View job


Claude PR Review

  • Read CLAUDE.md and project docs
  • Review the full PR diff (post-synchronize, latest commit bd10fce7)
  • Verify previously-flagged blocking issues
  • Check new changes since last review
  • Post final review

Summary

Problem: scenes (notably bloomgarden.dcl.eth) sometimes get stuck inside await runtimeInstance.UpdateScene(deltaTime) after a hiccup feeds a multi-second dt into JS. Scene stays at Running and dispose-on-teleport never fires.

Root cause vs symptom: PR is honest that the underlying V8/JS hang is obscure. The diff is layered:

  1. Starting state + synchronous cache add — real ordering fix. Cache add happens synchronously on the main thread before the JS init thread switch. State transitions NotStarted → Starting → Running as the scene progresses.
  2. V8 hang watchdog — escape valve: marks state JavaScriptError, calls engine.Interrupt().
  3. MAX_DELTA_TIME = 0.1f cap — pragmatic mitigation for the observed trigger.

Since the previous PASS review (commit bd10fce7 fix forgotten scene start & update):

  • Double-.Forget() eliminated — the previous iteration had scene.StartUpdateLoopAsync(fps, destroyCancellationToken).Forget() inside an already-.Forget()-ed StartAndUpdateSceneAsync, meaning any exception from StartUpdateLoopAsync itself was unobserved (not caught by the outer catch). The fix changes this to await scene.StartUpdateLoopAsync(...), so the outer catch (Exception e) { ReportHub.LogException(...) } now covers it. This was exactly what Mikhail requested.

All previously-resolved blocking issues remain fixed:

  • ✅ Watchdog UniTaskVoid has top-level try/catch (ignores OperationCanceledException, reports others)
  • ✅ Watchdog guards Set(JavaScriptError) with !IsNotRunningState() so it can't overwrite Disposing/Disposed
  • StartAsync watchdog CTS uses CreateLinkedTokenSource(ct)
  • ✅ All tickStartTimestamp accesses migrated to DCLInterlocked
  • OperationCanceledException caught separately in StartAndUpdateSceneAsync
  • UpdateLoopAsync returns early (not throws) when state is not Starting after watchdog-interrupted StartScene()
  • SyncedGroup allows Starting || Running to execute ECS systems
  • ✅ Consumer systems gate on Running for player assignment, readiness reporting, and deferred loading
  • GlobalDeferredLoadingSystem dead Contains check removed
  • SetTargetFPS body bug fixed
  • PlayerCRDTEntitiesHandlerSystem guards on state for both assign and cleanup paths
  • ✅ Four new tests covering Starting/Disposing state transitions

Blocking issues

None.

Non-blocking observations

  • Cache add before World.Add(entity, scene) (ControlSceneUpdateLoopSystem.cs:70-72): StartAndUpdateSceneAsync runs synchronously until DCLTask.SwitchToThreadPool(), which means scenesCache.Add fires before World.Add(entity, scene). Both happen on the same thread in the same frame before any other ECS system runs, so no structural race — but worth knowing for future readers.
  • ThreadStateException on early Dispose: if Dispose() runs between scenesCache.Add and StartUpdateLoopAsync, the NotStarted guard in StartUpdateLoopAsync throws ThreadStateException. With the new await this is now caught by the outer catch (Exception e) and logged — an improvement over the unobserved fault it was before.
  • Watchdog tests — no coverage added for the watchdog itself (interrupt path, threshold trip, state-transition guard). Not blocking.
  • QA note: confirm teleport / /reload / portable experiences do not regress with the Starting state gap.

REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Reworks scene runtime lifecycle (new Starting state, Starting/Running split in SyncedGroup), adds a V8 interrupt path, touches async ordering between JS init and IScenesCache registration, and modifies multiple ECS systems and scene readiness reporting.
QA_REQUIRED: YES

• Branch: fix/scene-lifecycle

@github-actions

Copy link
Copy Markdown
Contributor

🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging.

@DafGreco DafGreco left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️ PR reviewed and approved by QA on both platforms following instructions playing both happy and un-happy path

Regressions for this ticket had been performed in order to verify that the normal flow is working as expected:

  • [✔️ ] Backpack and wearables in world
  • [✔️ ] Emotes in world and in backpack
  • [ ✔️] Teleport with map/coordinates/Jump In
  • [ ✔️] Chat and multiplayer
  • [✔️ ] Profile card
  • [ ✔️] Settings

Teleported from one world to bloomgarden and to genesis plaza several times without any problems and could not repro the issue , also reloaded the platforms several times (+20) without any success on reproing the issue per-se 🚀
Played bloomgarden and attempted both repro states with no success , thanks team!

Link to the video since it takes more than 10 minutes of video

@lorux0
lorux0 enabled auto-merge (squash) May 14, 2026 16:20
@claude

claude Bot commented May 14, 2026

Copy link
Copy Markdown
Contributor

Claude finished @lorux0's task in 1m 36s —— View job


Claude finished @lorux0's task


Claude PR Review

  • Read CLAUDE.md and project docs
  • Review latest commit (3747424a fix test)
  • Verify all previously-resolved blocking issues remain fixed
  • Post final review

Summary

Problem: scenes (notably bloomgarden.dcl.eth) sometimes get stuck inside await runtimeInstance.UpdateScene(deltaTime) after a hiccup feeds a multi-second dt into JS. Scene stays at Running and dispose-on-teleport never fires.

Root cause vs symptom: PR is honest that the underlying V8/JS hang is obscure. The diff is layered:

  1. Starting state + synchronous cache add — real ordering fix. Cache add happens synchronously on the main thread before the JS init thread switch.
  2. V8 hang watchdog — escape valve: marks state JavaScriptError, calls engine.Interrupt().
  3. MAX_DELTA_TIME = 0.1f cap — pragmatic mitigation for the observed trigger.

Latest commit (3747424a fix test) adds // IGNORE_LINE_WEBGL_SYSTEM_TASKS_SAFETY_FLAG to the using System.Threading.Tasks; directive in SceneFacade.cs. This correctly suppresses a project WebGL-safety lint warning: the import is needed only for ValueTask<bool> in IdleWhileRunningAsync, which is already gated behind #if !UNITY_WEBGL — so the flag is legitimate and accurate.

All blocking issues resolved in prior commits remain fixed:

  • ✅ Watchdog UniTaskVoid has top-level try/catch (ignores OperationCanceledException, reports others)
  • ✅ Watchdog guards Set(JavaScriptError) with !IsNotRunningState() — cannot overwrite Disposing/Disposed
  • StartAsync watchdog CTS uses CreateLinkedTokenSource(ct)
  • ✅ All tickStartTimestamp accesses migrated to DCLInterlocked
  • OperationCanceledException caught separately in StartAndUpdateSceneAsync
  • UpdateLoopAsync returns early (not throws) when state is not Starting after watchdog-interrupted StartScene()
  • await scene.StartUpdateLoopAsync(...) (not .Forget()) — exceptions handled by outer catch
  • SyncedGroup gates on Starting || Running
  • ✅ Consumer systems gate on Running for player assignment, readiness reporting, and deferred loading
  • GlobalDeferredLoadingSystem dead Contains check removed
  • SetTargetFPS body bug fixed
  • ✅ Four new tests covering Starting/Disposing state transitions in PlayerCRDTEntitiesHandlerSystem

Blocking issues

None.

Non-blocking observations

  • StartAndUpdateSceneAsync ThreadStateException on early Dispose (ControlSceneUpdateLoopSystem.cs:117): if Dispose() runs between scenesCache.Add (line 101-103) and StartUpdateLoopAsync, the NotStarted guard throws ThreadStateException. With the await, this is now caught by the outer catch (Exception e) and logged — an improvement over the unobserved fault it was before.
  • QA note: confirm teleport / /reload / portable experiences do not regress with the Starting state gap.
  • Watchdog tests — no coverage added for the watchdog itself. Not blocking.

REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Reworks scene runtime lifecycle (new Starting state, Starting/Running split in SyncedGroup), adds a V8 interrupt path, touches async ordering between JS init and IScenesCache registration, and modifies multiple ECS systems and scene readiness reporting.
QA_REQUIRED: YES

• Branch: fix/scene-lifecycle

@github-actions

Copy link
Copy Markdown
Contributor

🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging.

@lorux0
lorux0 merged commit 888a8f7 into dev May 14, 2026
14 checks passed
@lorux0
lorux0 deleted the fix/scene-lifecycle branch May 14, 2026 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants